Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove inspector feature #11

Closed

Conversation

johanhelsing
Copy link
Contributor

bevy-inspector-egui now works perfectly fine through Reflect, so no need
to also derive inspector options when we don't use any of its
annotations.

depends on #10

@johanhelsing johanhelsing mentioned this pull request Nov 2, 2023
bevy-inspector-egui now works perfectly fine through Reflect, so no need
to also derive inspector options when we don't use any of its
annotations.
@johanhelsing johanhelsing force-pushed the remove-inspector-feature branch from f2c0fc1 to e223345 Compare November 2, 2023 08:37
@SergioRibera
Copy link
Owner

I really appreciate your contribution and interest in the project, but I'm afraid that this feature is really to have the least amount of dependencies in development mode, to make the resulting binary lighter, reduce compile times and in turn activate the debug processes that may exist (now that I think about it, it would be interesting to add borders to the interaction areas when the feature is activated for example)

#[cfg(feature = "serialize")]
use serde::{Deserialize, Serialize};

#[derive(Reflect, Clone, Copy, Debug, Default, PartialEq, Eq)]
#[cfg_attr(feature = "inspect", derive(InspectorOptions))]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could remove this part since, as you say, it now takes advantage of the reflect, but I don't think we should remove the feature

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what's the point of the feature if it doesn't do anything?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid having too many dependencies when compiling in development and give the freedom to choose what to use. Think that compiling in Rust already takes too much time to have to compile things that I don't need, for example, not everyone needs to have the type inspection of this library and when they need it they just add the feature :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's a development-only feature? I still don't see for this would be useful for users of the crate, though...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not everyone needs to have the type inspection of this library and when they need it they just add the feature :D

You understand that users of this crate won't get the dev-dependencies, even while they're developing, right?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You understand that users of this crate won't get the dev-dependencies, even while they're developing, right?

Yes, but precisely controlling the dependencies through the features allows them to choose in which environment they want to have it available, there will be those who want it in production, those who want it in development and those who do not want it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what does the feature do for them? They can't use it without also adding bevy-inspector-egui to their dependencies or dev-dependencies section? It does nothing except increase build times?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants